Also, I think the title should include the issue number in front!

@supermii2 Remember to add the issue number in the front of the title!

The following links are for previewing this pull request:

• Dashboard Preview: https://dashboard-2125-pr-reposense-reposense.surge.sh
• Docs Preview: https://docs-2125-pr-reposense-reposense.surge.sh

Thanks for your work on this! I believe most of the raw .ts files are still lacking return types - it might be too much for one PR and I'm ok with having this PR focus on just the functions in .vue files! In that case, we might want to update the title of this PR to reflect that, and change "Fixes" to "Part of" to prevent this PR from closing the entire issue.

Also as Jonas mentioned, it might be worth looking into if there's an eslint rule that can help enforce the return types! I believe it's possible to only enable the rule for .vue files too, and we can proceed to enable it for general .ts files once that portion has been completed in a separate PR.

Suggestion added, thank you!

Thanks for your changes @supermii2! If we are going ahead with the plan of splitting up the issue into two PRs (one for typescript functions inside Vue components, and one for functions in raw .ts files), can I trouble you to remove the changes to brokenLinkMixin.ts and store.ts in this PR? You can include them in the PR for the raw .ts files.

Additionally, could you update the title of this PR and the action statement (Let's... part of the commit message) to reflect that this PR is only adding return types to functions within Vue components?

The linting rule(s) can be added in a separate PR too, I've made an issue to track it in the meantime: #2156

Also as Jonas mentioned, it might be worth looking into if there's an eslint rule that can help enforce the return types! I believe it's possible to only enable the rule for .vue files too, and we can proceed to enable it for general .ts files once that portion has been completed in a separate PR

Hi, I've had to change the linting rules for unused variables to use the one built for typescript because it was creating false positives. Thank you!

I'll revert the changes done to *.ts in the commit after this comment.

Also as Jonas mentioned, it might be worth looking into if there's an eslint rule that can help enforce the return types! I believe it's possible to only enable the rule for .vue files too, and we can proceed to enable it for general .ts files once that portion has been completed in a separate PR

Hi, I've had to change the linting rules for unused variables to use the one built for typescript because it was creating false positives. Thank you!

I'll revert the changes done to *.ts in the commit after this comment.

Could you elaborate (maybe with some examples) regarding the false positives, and how changing the rule fixes it? Also, could you check on the grammar of the final sentence in the proposed commit message? Thanks!

@supermii2 bump on this, I think this PR is almost complete, just need some small changes and we can merge this in

Could you elaborate (maybe with some examples) regarding the false positives, and how changing the rule fixes it? Also, could you check on the grammar of the final sentence in the proposed commit message? Thanks!

    sortingFunction(): (a: Commit, b: Commit) => number {
      const commitSortFunction = this.commitsSortType === CommitsSortType.Time
        ? (commit: Commit): string => commit.date
        : (commit: Commit): number => commit.insertions;

      return (a: Commit, b: Commit): number => (this.toReverseSortedCommits ? -1 : 1)
        * window.comparator(commitSortFunction)(a, b);
    },

Here's a potential false positive found in c-zoom.vue. The default no-unused-vars marks a and b used in the return type as unused variables, which is fixed by using the typescript version.

Could you elaborate (maybe with some examples) regarding the false positives, and how changing the rule fixes it? Also, could you check on the grammar of the final sentence in the proposed commit message? Thanks!

    sortingFunction(): (a: Commit, b: Commit) => number {
      const commitSortFunction = this.commitsSortType === CommitsSortType.Time
        ? (commit: Commit): string => commit.date
        : (commit: Commit): number => commit.insertions;

      return (a: Commit, b: Commit): number => (this.toReverseSortedCommits ? -1 : 1)
        * window.comparator(commitSortFunction)(a, b);
    },

Here's a potential false positive found in c-zoom.vue. The default no-unused-vars marks a and b used in the return type as unused variables, which is fixed by using the typescript version.

Got it, thanks for clarifying! There's now merge conflicts introduced by #2132 that have to be resolved before we can merge this PR. Also as mentioned, could you check on the grammar of the final sentence in the proposed commit message? Thanks!

Hi @supermii2, I added the missing return types for the new file (c-zoom-commit-message) & updated the proposed commit message to move this PR along for now.

Please take note for future that whenever you finish the requested changes, do re-request a review and/or let us know the PR is ready for review again.

Thanks again for your work on this PR!

Thank you!

The following links are for previewing this pull request:

• Dashboard Preview: https://dashboard-2125-pr-reposense-reposense.surge.sh
• Docs Preview: https://docs-2125-pr-reposense-reposense.surge.sh